Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support IP Type of Service #13606

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Support IP Type of Service #13606

wants to merge 1 commit into from

Conversation

orgads
Copy link
Contributor

@orgads orgads commented May 12, 2024

Adds a --type-of-service option the command line tool for setting the TOS IPv4 header field.

CMake/CurlTests.c Outdated Show resolved Hide resolved
@github-actions github-actions bot added the CI Continuous Integration label May 12, 2024
@orgads orgads force-pushed the ip-tos branch 5 times, most recently from ddc3935 to 2c5db47 Compare May 12, 2024 13:13
@orgads
Copy link
Contributor Author

orgads commented May 12, 2024

I don't know what to do with the remaining tests that fail. Can you please advise?

@bagder
Copy link
Member

bagder commented May 12, 2024

Why is a new libcurl option needed? Can't CURLOPT_SOCKOPTFUNCTION already do this?

@bagder
Copy link
Member

bagder commented May 12, 2024

This is a IPv4-only feature. What happens for IPv6 sockets and how is a user supposed to know when this works or not?

Can you please explain your use case for this?

src/tool_getparam.c Outdated Show resolved Hide resolved
@orgads
Copy link
Contributor Author

orgads commented May 12, 2024

Why is a new libcurl option needed? Can't CURLOPT_SOCKOPTFUNCTION already do this?

We use it with the command-line interface. And if I understand correctly, CURLOPT_SOCKOPTFUNCTION is for user applications, not for internal use by curl, right?

@orgads
Copy link
Contributor Author

orgads commented May 12, 2024

This is a IPv4-only feature. What happens for IPv6 sockets and how is a user supposed to know when this works or not?

Can you please explain your use case for this?

We need it for applying QoS for IP packets. All the other parts of our application support this, and we've been patching curl for long time to add support. Now we'd like to upstream our patches (2 more will follow :)).

I noticed that another used proposed a similar change in #7762, but it was abandoned. I took the user-friendly names from that PR.

@bagder bagder added the feature-window A merge of this requires an open feature window label May 13, 2024
@bagder
Copy link
Member

bagder commented May 13, 2024

Why is a new libcurl option needed? Can't CURLOPT_SOCKOPTFUNCTION already do this?

We use it with the command-line interface. And if I understand correctly, CURLOPT_SOCKOPTFUNCTION is for user applications, not for internal use by curl, right?

You add CURLOPT_TYPE_OF_SERVICE in this PR. It is not for the command line tool, that's a new option for libcurl and I wonder why you need it.

src/tool_getparam.c Outdated Show resolved Hide resolved
src/tool_getparam.c Outdated Show resolved Hide resolved
@orgads
Copy link
Contributor Author

orgads commented May 13, 2024

Why is a new libcurl option needed? Can't CURLOPT_SOCKOPTFUNCTION already do this?

We use it with the command-line interface. And if I understand correctly, CURLOPT_SOCKOPTFUNCTION is for user applications, not for internal use by curl, right?

You add CURLOPT_TYPE_OF_SERVICE in this PR. It is not for the command line tool, that's a new option for libcurl and I wonder why you need it.

It is used in tool_operate.c. If I understand the relations correctly, this sets the library options according to the command-line arguments. So do you suggest using CURLOPT_SOCKOPTFUNCTION there? I don't see any other option that uses it there, why should this one be different?

@orgads orgads force-pushed the ip-tos branch 2 times, most recently from bcb0e25 to 1af883c Compare May 13, 2024 07:37
@bagder
Copy link
Member

bagder commented May 13, 2024

So do you suggest using CURLOPT_SOCKOPTFUNCTION there?

I do. I don't see why libcurl needs another option when you can already accomplish the same thing using an existing option.

@orgads
Copy link
Contributor Author

orgads commented May 13, 2024

So do you suggest using CURLOPT_SOCKOPTFUNCTION there?

I do. I don't see why libcurl needs another option when you can already accomplish the same thing using an existing option.

The next feature I plan to add is setting VLAN priority, which is another setsockopt (setsockopt(sockfd, SOL_SOCKET, SO_PRIORITY, (char*)&priority, sizeof(priority))). How can I support both options? There is only one SOCKOPTFUNCTION entry.

@bagder
Copy link
Member

bagder commented May 13, 2024

There is only one SOCKOPTFUNCTION entry.

So do both in the same callback?

m4/curl-functions.m4 Outdated Show resolved Hide resolved
@orgads orgads force-pushed the ip-tos branch 2 times, most recently from dce54bd to e6a7c46 Compare May 13, 2024 08:33
@orgads
Copy link
Contributor Author

orgads commented May 13, 2024

There is only one SOCKOPTFUNCTION entry.

So do both in the same callback?

Done. Is this what you meant?

@orgads
Copy link
Contributor Author

orgads commented May 13, 2024

The failing test in quiche looks like a flaky test, I doubt it's related to my changes.

@icing
Copy link
Contributor

icing commented May 13, 2024

The failing test in quiche looks like a flaky test, I doubt it's related to my changes.

Yes, we try to hunt that down for some time. Sorry about that.

src/tool_operate.c Outdated Show resolved Hide resolved
src/tool_operate.c Outdated Show resolved Hide resolved
src/tool_operate.c Outdated Show resolved Hide resolved
docs/cmdline-opts/type-of-service.md Outdated Show resolved Hide resolved
docs/cmdline-opts/type-of-service.md Outdated Show resolved Hide resolved
docs/options-in-versions Outdated Show resolved Hide resolved
docs/cmdline-opts/type-of-service.md Outdated Show resolved Hide resolved
@orgads orgads force-pushed the ip-tos branch 7 times, most recently from c290eb0 to 0d692fa Compare May 19, 2024 07:05
Copy link
Contributor

@rpigott rpigott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also interested in this feature, so thanks for writing it up!

src/tool_getparam.c Outdated Show resolved Hide resolved
src/tool_getparam.c Show resolved Hide resolved
docs/cmdline-opts/type-of-service.md Show resolved Hide resolved
@orgads orgads force-pushed the ip-tos branch 6 times, most recently from 0cfe17e to 23ece88 Compare May 23, 2024 07:42
@orgads
Copy link
Contributor Author

orgads commented May 23, 2024

@bagder Is this good to go, now that 8.8 was released?

@orgads orgads requested review from bagder and vszakats May 23, 2024 14:08
Copy link
Member

@bagder bagder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We cannot merge this anyway until the feature window opens on June 1st if everything goes well.

docs/cmdline-opts/type-of-service.md Outdated Show resolved Hide resolved
src/tool_operate.c Outdated Show resolved Hide resolved
src/tool_operate.c Outdated Show resolved Hide resolved
src/tool_operate.c Outdated Show resolved Hide resolved
@orgads
Copy link
Contributor Author

orgads commented May 24, 2024

Is the msys2 with debug flaky? I couldn't find anything in the log that looks related to my changes.

@vszakats
Copy link
Member

@orgads: Yes, the MSYS2 mingw-w64 ones sometimes hang in tests. I'm tracking the problem here: #13599 (comment). Could not yet figure out which test is prone to hanging though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous Integration cmdline tool feature-window A merge of this requires an open feature window libcurl API tests
Development

Successfully merging this pull request may close these issues.

None yet

5 participants